feat: Allow filtering by multiple tags [FC-0040] #945

bradenmacdonald · 2024-04-11T00:14:08Z

Description

As of #918 , the content search only allows filtering the results by one tag at a time, which is a limitation of Instantsearch.

So with this PR, I have replaced our usage of Instantsearch + instant-meilisearch with just direct usage of Meilisearch. Not only does this simplify the code and make our MFE bundle size smaller, but it allows us much more control over how the tags filtering works, so that I can implement searching by multiple tags.

(Trying to modify Instantsearch to do that was too difficult, given the complexity of its codebase).

Related ticket: openedx/modular-learning#201

Before

Only one tag can be selected at a time.
You must select a parent tag before you can select a child filter. The parent tag(s) shows a checkmark even though only the child tag's filter is active.
You cannot tell if a tag has children or not until you click on it.

After

You can select multiple tags. They are filtered using an AND filter.
You can select a child filter without selecting the parent.
You can tell which tags have children or not.
Several dependencies (instantsearch and its depdencies) have been removed.

TODO

Pagination of search results!
Display a message if tags list may be incomplete - either due to 100 facet limit or 1,000 results keyword limit
Publish the edx-platform PR
Tests

Testing instructions

You must first check out feat: update meilisearch configuration needed for tags filter [FC-0040] edx-platform#34534 and run ./manage.py cms reindex_studio --experimental in your CMS container.
Go to the Course Authoring MFE, e.g. http://apps.local.overhang.io:2001/course-authoring/home
Optionally, browse to a specific course.
Click on the search icon in the header:
Try changing the "Type" filter, and entering keywords in both the overall search keywords field and the tag search keywords fields. Notice how the list of tags dynamically changes to reflect the current results of the search.
Try a search that matches a lot of content to check that pagination is working.

Other information

Private ref: FAL-3704

Depends on: #918

openedx-webhooks · 2024-04-11T00:14:13Z

Thanks for the pull request, @bradenmacdonald! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

rpenido · 2024-04-12T19:15:05Z

Hi @bradenmacdonald! If you have this ready before EOD Monday, could you ping me so I can include these changes in the sandbox? Thanks!

bradenmacdonald · 2024-04-12T19:37:54Z

@rpenido Sure!

bradenmacdonald · 2024-04-14T02:07:02Z

@rpenido This is ready! At least, ready enough for the sandbox. It still needs a lot of refinement before it's ready for code review.

For your convenience, I merged it with your sandbox branch on a new branch called taxonomy-sandbox-20240412-b. There were quite a few conflicts so I hope I didn't break too much of your work. I think the breadcrumbs may need some fixup.

rpenido · 2024-04-17T04:26:32Z

@rpenido This is ready! At least, ready enough for the sandbox. It still needs a lot of refinement before it's ready for code review.

For your convenience, I merged it with your sandbox branch on a new branch called taxonomy-sandbox-20240412-b. There were quite a few conflicts so I hope I didn't break too much of your work. I think the breadcrumbs may need some fixup.

Ack. The current sandbox is running based on this branch. Tomorrow, I will take a look to fix the breadcrumbs and see if there are more things to fix. Thank you @bradenmacdonald !

Edit: The tag filter is not working in the sandbox:

Attribute `tags.taxonomy` is not facet-searchable. Available facet-searchable attributes are: `block_type, context_key, org, tags, type`. To make it facet-searchable add it to the `filterableAttributes` index settings.

We probably need to fix this while creating the index.

bradenmacdonald · 2024-04-17T17:13:19Z

@rpenido Yeah, I forgot about the edx-platform changes required :/ I pinged you about it on MatterMost. It just needs a few lines of code changed and the index regenerated: openedx/edx-platform#34534

bradenmacdonald · 2024-04-18T04:01:00Z

@rpenido This is ready for review.

bradenmacdonald · 2024-04-18T04:04:13Z

src/search-modal/SearchModal.test.jsx

@@ -67,17 +67,9 @@ describe('<SearchModal />', () => {
    expect(await findByText('Start searching to find content')).toBeInTheDocument();
  });

-  it('should render the spinner while the config is loading', () => {


The spinner is no longer needed. Now the search UI displays immediately without waiting for the connection details, which is a better user experience.

codecov · 2024-04-18T04:10:38Z

Codecov Report

Attention: Patch coverage is 89.18919% with 28 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (master@0814022). Click here to learn what that means.
Report is 2 commits behind head on master.

Files	Patch %	Lines
src/search-modal/FilterByTags.jsx	86.95%	5 Missing and 1 partial ⚠️
src/search-modal/Highlight.jsx	50.00%	5 Missing ⚠️
src/search-modal/data/apiHooks.js	88.37%	4 Missing and 1 partial ⚠️
src/search-modal/SearchResults.jsx	70.00%	2 Missing and 1 partial ⚠️
src/search-modal/data/api.js	96.73%	3 Missing ⚠️
src/search-modal/manager/SearchManager.js	87.50%	3 Missing ⚠️
src/search-modal/FilterByBlockType.jsx	71.42%	1 Missing and 1 partial ⚠️
src/search-modal/SearchKeywordsField.jsx	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master     #945   +/-   ##
=========================================
  Coverage          ?   92.00%           
=========================================
  Files             ?      636           
  Lines             ?    11273           
  Branches          ?     2436           
=========================================
  Hits              ?    10372           
  Misses            ?      868           
  Partials          ?       33

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rpenido

Hi @bradenmacdonald !

I read through the code and only have some nits and syntax suggestions. Good work here!

While testing, I found some errors while typing in the tag search input.

The failed request is bellow:

curl 'http://meilisearch.local.edly.io:7700/indexes/tutor_studio_content/search' --compressed -X POST -H 'User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:124.0) Gecko/20100101 Firefox/124.0' -H 'Accept: */*' -H 'Accept-Language: pt-BR,pt;q=0.8,en-US;q=0.5,en;q=0.3' -H 'Accept-Encoding: gzip, deflate' -H 'Referer: http://apps.local.edly.io:2001/' -H 'Authorization: Bearer eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJhcGlLZXlVaWQiOiJhOGE1NGM2Ny0zZTkwLTQ2YmItYWMzMC03NWFkZDZiOWJhMTkiLCJzZWFyY2hSdWxlcyI6eyJ0dXRvcl9zdHVkaW9fY29udGVudCI6e319LCJleHAiOjE3MTQwNTEzNzV9.hRsgsRseHHV3KuX65VUTdlDmZurEx8orBwY-pp22jjM' -H 'Content-Type: application/json' -H 'X-Meilisearch-Client: Meilisearch JavaScript (v0.38.0)' -H 'Origin: http://apps.local.edly.io:2001' -H 'Connection: keep-alive' --data-raw '{"q":"e","filter":[],"attributesToSearchOn":["tags.taxonomy","tags.level0","tags.level1","tags.level2","tags.level3"],"attributesToRetrieve":["tags"],"limit":1000}'

And the response:

{"message":"Attribute `tags.level3` is not searchable. Available searchable attributes are: `block_id, content, content.background_image_description, content.capa_content, content.html_content, content.item_0_display_name, content.item_0_image_description, content.item_1_display_name, content.item_1_image_description, content.item_2_display_name, content.item_2_image_description, content.item_3_display_name, content.item_3_image_description, content.item_4_display_name, content.item_4_image_description, content.problem_types, content.prompt, content.question_text, content.title, content.zone_0_description, content.zone_0_display_name, content.zone_1_description, content.zone_1_display_name, content.zone_2_description, content.zone_2_display_name, display_name, tags, tags.level0, tags.level1, tags.level2, tags.taxonomy`.","code":"invalid_search_attributes_to_search_on","type":"invalid_request","link":"https://docs.meilisearch.com/errors#invalid_search_attributes_to_search_on"}

I find it odd because it is working (for me) in the sandbox without adding the tags.level* to the searchable fields. Also, @yusuf-musleh had the same error in the sandbox (and couldn't reproduce it also).

src/search-modal/SearchResult.jsx

src/search-modal/Highlight.jsx

src/search-modal/data/api.js

src/search-modal/data/apiHooks.js

bradenmacdonald · 2024-04-18T16:28:42Z

@rpenido

I suspect that that error occurs only if you don't actually have any level3 tags in your search index. In other words, it's not so much the setting, but it's about what content/tags you have. I will try deleting all my level3 tags from the database and see if I can reproduce it.

Co-authored-by: Rômulo Penido <[email protected]>

bradenmacdonald · 2024-04-19T18:19:12Z

@xitij2000 This is ready for your review and (once approved) merge.

xitij2000

The code seems to be working as expected and I don't have any other specific feedback, but I did notice that the meilisearch API is being directly used.

I don't know about meilisearch to know if this is a problem, but doesn't exposing it directly mean that anyone can run queries against the server and get access to data that's been indexed but shouldn't be accessible to them?

bradenmacdonald · 2024-04-23T16:42:00Z

@xitij2000 Thanks! Good question, but the answer is no - we give each user a restricted API key that only allows them to search content that they're allowed to see. You can see openedx/edx-platform#34471 if you're interested in the details.

This approach is much faster since the CMS/LMS isn't needed to act as a proxy for the search engine, and it reduces load on the edxapp django IDA.

bradenmacdonald · 2024-04-23T21:16:57Z

@xitij2000 Could you please merge this then? :)

openedx-webhooks · 2024-04-24T03:45:33Z

@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

bradenmacdonald requested a review from a team as a code owner April 11, 2024 00:14

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Apr 11, 2024

bradenmacdonald force-pushed the braden/meilisearch-direct branch from bb13328 to 71024e9 Compare April 11, 2024 16:15

feat: make "refine by tags" menu wider

559ad99

bradenmacdonald force-pushed the braden/meilisearch-direct branch from 71024e9 to e1abaf7 Compare April 12, 2024 19:37

feat: working tags filter with multi-select support

308b5aa

bradenmacdonald force-pushed the braden/meilisearch-direct branch from 8b26071 to 308b5aa Compare April 14, 2024 01:43

bradenmacdonald added 2 commits April 16, 2024 13:58

feat: Infinite loading of additional pages of results

9d09ede

feat: display a warning if we're not able to load all the tags

2bc510a

bradenmacdonald mentioned this pull request Apr 17, 2024

feat: update meilisearch configuration needed for tags filter [FC-0040] openedx/edx-platform#34534

Closed

bradenmacdonald added 2 commits April 17, 2024 11:52

chore: update with latest master

04da447

feat: make icons consistent with the rest of Course-Authoring

2e2cdf7

bradenmacdonald force-pushed the braden/meilisearch-direct branch from 996fd2d to 2e2cdf7 Compare April 17, 2024 18:52

test: update tests

2cdbcd8

bradenmacdonald changed the title ~~feat: Allow filtering by multiple tags [WIP] [FC-0040]~~ feat: Allow filtering by multiple tags [FC-0040] Apr 18, 2024

fix: show an error when the search connection isn't working

bc7943c

bradenmacdonald force-pushed the braden/meilisearch-direct branch from 756aaa9 to bc7943c Compare April 18, 2024 04:00

bradenmacdonald commented Apr 18, 2024

View reviewed changes

docs: fix docstring

2a21832

rpenido reviewed Apr 18, 2024

View reviewed changes

fix: apply suggestions from code review

173e9bf

Co-authored-by: Rômulo Penido <[email protected]>

bradenmacdonald added 6 commits April 18, 2024 09:37

chore: better handling of highlighting markers

a0f9bf2

test: update tests

ceea96a

test: add test for retrieving child tags

9602c84

test: add a test for searching by keyword tag

5e9110b

perf: remove some unnecessary queries

32c78dd

perf: load tags filter data in parallel when possible

2fab892

bradenmacdonald force-pushed the braden/meilisearch-direct branch from 66e4122 to 2fab892 Compare April 19, 2024 18:12

xitij2000 approved these changes Apr 23, 2024

View reviewed changes

rpenido mentioned this pull request Apr 23, 2024

feat: redirect to unit page if the hit or its parent is a unit #957

Merged

xitij2000 merged commit c32462e into openedx:master Apr 24, 2024
6 checks passed

bradenmacdonald deleted the braden/meilisearch-direct branch June 13, 2024 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow filtering by multiple tags [FC-0040] #945

feat: Allow filtering by multiple tags [FC-0040] #945

bradenmacdonald commented Apr 11, 2024 •

edited

Loading

openedx-webhooks commented Apr 11, 2024 •

edited

Loading

rpenido commented Apr 12, 2024

bradenmacdonald commented Apr 12, 2024

bradenmacdonald commented Apr 14, 2024

rpenido commented Apr 17, 2024 •

edited

Loading

bradenmacdonald commented Apr 17, 2024

bradenmacdonald commented Apr 18, 2024

bradenmacdonald Apr 18, 2024

codecov bot commented Apr 18, 2024 •

edited

Loading

rpenido left a comment

bradenmacdonald commented Apr 18, 2024

bradenmacdonald commented Apr 19, 2024

xitij2000 left a comment

bradenmacdonald commented Apr 23, 2024 •

edited

Loading

bradenmacdonald commented Apr 23, 2024

openedx-webhooks commented Apr 24, 2024

feat: Allow filtering by multiple tags [FC-0040] #945

feat: Allow filtering by multiple tags [FC-0040] #945

Conversation

bradenmacdonald commented Apr 11, 2024 • edited Loading

Description

Before

After

TODO

Testing instructions

Other information

openedx-webhooks commented Apr 11, 2024 • edited Loading

rpenido commented Apr 12, 2024

bradenmacdonald commented Apr 12, 2024

bradenmacdonald commented Apr 14, 2024

rpenido commented Apr 17, 2024 • edited Loading

bradenmacdonald commented Apr 17, 2024

bradenmacdonald commented Apr 18, 2024

bradenmacdonald Apr 18, 2024

Choose a reason for hiding this comment

codecov bot commented Apr 18, 2024 • edited Loading

Codecov Report

rpenido left a comment

Choose a reason for hiding this comment

bradenmacdonald commented Apr 18, 2024

bradenmacdonald commented Apr 19, 2024

xitij2000 left a comment

Choose a reason for hiding this comment

bradenmacdonald commented Apr 23, 2024 • edited Loading

bradenmacdonald commented Apr 23, 2024

openedx-webhooks commented Apr 24, 2024

bradenmacdonald commented Apr 11, 2024 •

edited

Loading

openedx-webhooks commented Apr 11, 2024 •

edited

Loading

rpenido commented Apr 17, 2024 •

edited

Loading

codecov bot commented Apr 18, 2024 •

edited

Loading

bradenmacdonald commented Apr 23, 2024 •

edited

Loading